perf(net): isolate object request scheduling from cs_main - #7673
perf(net): isolate object request scheduling from cs_main#7673PastaPastaPasta wants to merge 1 commit into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
🕓 Queued for automated review — 2nd in line, estimated start in ~20 min (commit 66807a1)
|
Potential PR merge conflictsThis is advisory only. It does not block CI, but it marks PRs that will likely need a rebase depending on merge order. If this PR merges firstThese open PRs will likely need a rebase:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe change protects Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change moves inventory request tracking onto a dedicated lock so request scheduling no longer waits on the main chainstate lock, and adds benchmarks, tests, and documentation. Investigation of the flagged concerns found no correctness, concurrency, or build problems at the current head, so the change appears safe to merge with normal validation. Sequence Diagram(s)sequenceDiagram
participant BenchmarkPeer
participant PeerManagerImpl
participant TxRequestTracker
participant RPCObserver
BenchmarkPeer->>PeerManagerImpl: Announce inventory
PeerManagerImpl->>TxRequestTracker: Register request under m_object_request_mutex
BenchmarkPeer->>PeerManagerImpl: Return NOTFOUND
PeerManagerImpl->>TxRequestTracker: Complete request
RPCObserver->>PeerManagerImpl: Call getblockcount
PeerManagerImpl-->>RPCObserver: Return block count and latency sample
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 3.70% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 27 functions across 4 files. (2 skipped: 2 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
2 participants
Issue being fixed or feature implemented
Large object-inventory workloads hold
cs_mainwhile the request tracker selects candidates, advances its time state, and handles NOTFOUND responses. This stalls unrelated chainstate readers such asgetblockcount. Release-build stack sampling identifiedTxRequestTracker::GetRequestableand request-state transitions inSendMessagesas substantial work inside the critical section.Supersedes #6990. The object downloader was replaced by #5943, so this change works with the current tracker.
What was done?
Give the existing object request tracker its own mutex. Selection and tracker-only completion/cleanup can run without
cs_main; block scheduling, availability checks, and per-peer response authorization remain protected bycs_main. When both locks are needed, acquirecs_mainfirst. All tracker access sites and compiler lock annotations are updated.Add native inventory benchmarks, a reproducible regtest contention workload with an independent RPC observer process, an accounting test, and a regression test proving NOTFOUND can complete while another thread holds
cs_main.How Has This Been Tested?
Performance
Successful isolated comparison run (download
inventory-profile-resultsfor raw RPC samples, metrics, lock waits, build logs, and source/binary hashes). Ubuntu 24.04 GitHub-hosted runner; both binaries built with Clang,-O2 -g, and-DDEBUG_LOCKCONTENTION. The same benchmark code is built into both; onlynet_processing.cppdiffers from baselinec652c314a24c59599d987da3779fe1c610dc4851. The candidate source SHA256 isa94050f00e4282442ce3964f114ef6c8ba465133fad172f203cdf7eca1462629, matching this PR and the local safety builds.Three alternating baseline/candidate pairs per workload, eight rounds each. Every run requested every announcement (400,000 for large, 3,200 for small, and 160,000 for governance). Values below are medians of the three per-run measurements:
cs_main, msThe large-message worst RPC latency improved in every pair: baseline 96.83 / 107.25 / 108.17 ms, candidate 76.89 / 76.94 / 77.04 ms. Its long stalls are too rare for p99 alone to characterize them. Governance p99 was 8.94 / 8.83 / 7.54 ms versus 6.06 / 5.82 / 5.80 ms.
Native INV → scheduling → NOTFOUND processing cost, median ns/inventory across three runs:
The benefit is reduced contention for independent chainstate readers. Native processing cost is essentially unchanged, while the governance socket workload completed about 16% slower; that workload includes P2P serialization, polling, and scheduling. This is a synthetic unknown-object/fallback workload, not evidence of higher live-network throughput. INV processing still holds
cs_main, and callers needing both locks can still wait on the tracker. The benchmark tool and reproduction instructions are included in this PR.Local validation on an Apple M4 Max, macOS 15 / Darwin 24.6, using prebuilt depends:
net_tests,txrequest_tests,governance_inv_tests: 33 cases and 287,437 assertions pass.notfound_does_not_wait_for_chainstatefails on baselinec652c314a24c59599d987da3779fe1c610dc4851at its completion assertion and passes on this implementation. Its bounded timeout is for failure cleanup; the invariant is completion while the other thread still holdscs_main.halt_on_error=1): transaction download, both deadlock variants, and concurrent inventory/RPC workloads for sporks and governance votes (32,000 announcements each) pass. The sanitizer build omits wallet support, so the full governance functional test could not start there; it passed in the normal build.Safety review:
NodesSnapshotkeeps the current node alive during message processing. Selection returns copied inventories; noCNodeStateor chain-index reference escapes its lock. The send path rechecks availability undercs_main. Typed accounting, request limits, delays/expiry, fallback behavior, consume-once semantics, and late-response grace retain their existing rules. The tests cover duplicates, shared hashes across types, unsolicited responses, fallback, reannouncement, and disconnect cleanup. Sanitizers and tests establish evidence for covered paths, not exhaustive proof over all interleavings.Breaking Changes
None.
Checklist:
This pull request was created by Codex.